Skip to content

SONARJAVA-6605 Implement S9017: Mockito stubbing chains should be complete#5782

Merged
NoemieBenard merged 8 commits into
masterfrom
nb/sonarjava-6605-implement-S9017
Jul 14, 2026
Merged

SONARJAVA-6605 Implement S9017: Mockito stubbing chains should be complete#5782
NoemieBenard merged 8 commits into
masterfrom
nb/sonarjava-6605-implement-S9017

Conversation

@NoemieBenard

@NoemieBenard NoemieBenard commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Summary by Gitar

  • New rule implementation:
    • Implemented S9017 to detect incomplete Mockito stubbing chains using when() or do*() methods.
    • Added rule metadata, documentation, and comprehensive test cases in MockitoStubbingChainCheckSample.java.

This will update automatically on new commits.

@hashicorp-vault-sonar-prod

hashicorp-vault-sonar-prod Bot commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

SONARJAVA-6605

@NoemieBenard NoemieBenard marked this pull request as ready for review July 13, 2026 14:13
Comment on lines +89 to +94
Boolean previous = isChained;
isChained = true;
scan(mit.methodSelect());
// Scan lambda/anonymous-class arguments as independent chains (reset isChained so
// incomplete stubs inside their bodies are detected, but they are not part of this chain)
isChained = null;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using an instance variable isChained and calling scan directly doesn't fill clean. Couldn't we have a recursive version of visitMethodInvocation that takes the equivalent of isChained as an argument.

@romainbrenguier romainbrenguier left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks better. Thanks

@gitar-bot

gitar-bot Bot commented Jul 14, 2026

Copy link
Copy Markdown
Code Review ✅ Approved 3 resolved / 3 findings

Implements S9017 to detect incomplete Mockito stubbing chains with full test coverage, addressing previous issues regarding redundant parameters, variable assignments, and lambda scoping.

✅ 3 resolved
Edge Case: False positive: stub assigned to pre-declared variable

📄 java-checks/src/main/java/org/sonar/java/checks/tests/MockitoStubbingChainCheck.java:66-68 📄 java-checks/src/main/java/org/sonar/java/checks/tests/MockitoStubbingChainCheck.java:91-93
The check suppresses reporting when a stub is stored in a variable by making visitVariable a no-op, which skips the initializer of a VariableTree (e.g. OngoingStubbing<User> stub = when(repo.findUser(42));). However, this only covers variable declarations. When the stub is stored via a plain assignment to a previously declared variable, e.g.:

OngoingStubbing<User> stub;
stub = when(repo.findUser(42));
stub.thenReturn(new User("Alice"));

the RHS is reached through AssignmentExpressionTree, not VariableTree. visitMethodInvocation then sees when(...) as the top-level invocation with isChained == null and raises a false positive, even though the stubbing is completed later. Because S9017 is a Critical/HIGH-reliability rule, a false positive on valid test code erodes trust.

Consider also skipping assignment right-hand sides (override visitAssignmentExpression similarly, or detect that the invocation is an assignment RHS) so the behavior matches the variable-declaration case. Add a test covering the reassignment pattern.

Edge Case: Incomplete stubbings inside lambda/anonymous args not detected

📄 java-checks/src/main/java/org/sonar/java/checks/tests/MockitoStubbingChainCheck.java:82-83
visitMethodInvocation only recurses into mit.methodSelect() and intentionally does not scan the arguments. While this correctly avoids reprocessing chain receivers, it also means any code nested inside argument expressions is never visited. As a result, incomplete stubbings inside lambda bodies or anonymous classes passed as arguments are missed, for example:

assertThrows(RuntimeException.class, () -> {
  when(repo.findUser(1)); // incomplete, not flagged
  service.deleteUser(1);
});

This is a false negative that reduces the rule's effectiveness in common test patterns (lambdas passed to assertThrows, doAnswer, etc.). If this limitation is intentional, consider documenting it; otherwise, scan the bodies of lambda/anonymous-class arguments (while still not treating them as part of the current chain, i.e. reset isChained to null when descending into them).

Quality: Redundant isChained parameter in isIncompleteStubbing

📄 java-checks/src/main/java/org/sonar/java/checks/tests/MockitoStubbingChainCheck.java:97-110
isIncompleteStubbing is only ever called at line 98 inside a !isChained guard, so isChained is always false there and the if (isChained) return false; branch (lines 108-110) is dead code. Drop the parameter and that early-return to simplify the method and avoid confusing maintainers into thinking the guard is reachable.

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change the behavior for this request:

Auto-apply Compact
gitar auto-apply:on         
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar

@sonarqube-next

Copy link
Copy Markdown

@NoemieBenard NoemieBenard merged commit 5c45a78 into master Jul 14, 2026
15 checks passed
@NoemieBenard NoemieBenard deleted the nb/sonarjava-6605-implement-S9017 branch July 14, 2026 08:48
@gitar-bot

gitar-bot Bot commented Jul 14, 2026

Copy link
Copy Markdown
Code Review ✅ Approved 3 resolved / 3 findings

Implements S9017 to detect incomplete Mockito stubbing chains with full test coverage, addressing previous issues regarding redundant parameters, variable assignments, and lambda scoping.

✅ 3 resolved
Edge Case: False positive: stub assigned to pre-declared variable

📄 java-checks/src/main/java/org/sonar/java/checks/tests/MockitoStubbingChainCheck.java:66-68 📄 java-checks/src/main/java/org/sonar/java/checks/tests/MockitoStubbingChainCheck.java:91-93
The check suppresses reporting when a stub is stored in a variable by making visitVariable a no-op, which skips the initializer of a VariableTree (e.g. OngoingStubbing<User> stub = when(repo.findUser(42));). However, this only covers variable declarations. When the stub is stored via a plain assignment to a previously declared variable, e.g.:

OngoingStubbing<User> stub;
stub = when(repo.findUser(42));
stub.thenReturn(new User("Alice"));

the RHS is reached through AssignmentExpressionTree, not VariableTree. visitMethodInvocation then sees when(...) as the top-level invocation with isChained == null and raises a false positive, even though the stubbing is completed later. Because S9017 is a Critical/HIGH-reliability rule, a false positive on valid test code erodes trust.

Consider also skipping assignment right-hand sides (override visitAssignmentExpression similarly, or detect that the invocation is an assignment RHS) so the behavior matches the variable-declaration case. Add a test covering the reassignment pattern.

Edge Case: Incomplete stubbings inside lambda/anonymous args not detected

📄 java-checks/src/main/java/org/sonar/java/checks/tests/MockitoStubbingChainCheck.java:82-83
visitMethodInvocation only recurses into mit.methodSelect() and intentionally does not scan the arguments. While this correctly avoids reprocessing chain receivers, it also means any code nested inside argument expressions is never visited. As a result, incomplete stubbings inside lambda bodies or anonymous classes passed as arguments are missed, for example:

assertThrows(RuntimeException.class, () -> {
  when(repo.findUser(1)); // incomplete, not flagged
  service.deleteUser(1);
});

This is a false negative that reduces the rule's effectiveness in common test patterns (lambdas passed to assertThrows, doAnswer, etc.). If this limitation is intentional, consider documenting it; otherwise, scan the bodies of lambda/anonymous-class arguments (while still not treating them as part of the current chain, i.e. reset isChained to null when descending into them).

Quality: Redundant isChained parameter in isIncompleteStubbing

📄 java-checks/src/main/java/org/sonar/java/checks/tests/MockitoStubbingChainCheck.java:97-110
isIncompleteStubbing is only ever called at line 98 inside a !isChained guard, so isChained is always false there and the if (isChained) return false; branch (lines 108-110) is dead code. Drop the parameter and that early-return to simplify the method and avoid confusing maintainers into thinking the guard is reachable.

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change the behavior for this request:

Auto-apply Compact
gitar auto-apply:on         
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants